Update(ModelTime): refactor ModelTime and add new features #2367

jlarsen-usgs · 2024-11-13T22:33:33Z

refactor call signature to accept perlen, nstp, and tsmult as parameters
added methods:
- set_start_datetime(): allows user to set/reset the start_datetime
- set_time_units(): allows user to set/reset the time_units
- timeunits_from_user_input(): uses fuzzy string logic to parse user supplied time units
- datetime_from_user_input(): converts many types of datetime representation to a standard datetime object
- get_totim(): allows user to get the totim value at the end of a stress period or stress period/time step combination
- get_datetime(): allows user to get the datetime value at the end of a stress period or stress period/time step combination
- intersect(): method that allows for time intersection. Method accepts datetime representations or totim and returns either the zero based stress period or the stress period and time step combination
added testing for ModelTime
updated FloPy model classes for ModelTime changes

* refactor call signature to accept perlen, nstp, and tsmult as parameters * added methods: - `set_start_datetime()`: allows user to set/reset the start_datetime - `set_time_units()`: allows user to set/reset the time_units - `timeunits_from_user_input()`: uses fuzzy string logic to parse user supplied time units - `datetime_from_user_input()`: converts many types of datetime representation to a standard datetime object - `get_totim()`: allows user to get the totim value at the end of a stress period or stress period/time step combination - `get_datetime()`: allows user to get the datetime value at the end of a stress period or stress period/time step combination - `intersect()`: method that allows for time intersection. Method accepts datetime representations or totim and returns either the zero based stress period or the stress period and time step combination * added testing for ModelTime * updated FloPy model classes for ModelTime changes

* add get_datetime_string() method to ModelTime to format ISO 8601 compliant date times

wpbonelli · 2024-11-14T15:00:57Z

will this close #1915?

jlarsen-usgs · 2024-11-14T17:16:40Z

@wpbonelli

This will close #1915. The TemporalReference class had very few uses and it's functionality has been 1) replaced and 2) improved on in this PR.

langevin-usgs

Hey Josh, I had a bunch of little comments, many just about terminology. Just thought I'd bring them up for discussion before they get burned in. I think what you've done here will be widely used.

One thing to consider: we have adaptive time stepping now, which doesn't fit the mold of nstp/tsmult. Not sure how we should handle it, but I think we should leave space for it somehow. Unfortunately, we don't know the time stepping information until after the simulation runs, but we could have ModelTime load the information. We can modify ATS to write time step information, which might be helpful.

Is there a way to get a list of datetimes that correspond to the end of each time step? Seems like this would be great for making plots?

langevin-usgs · 2024-11-14T22:04:13Z

flopy/discretization/modeltime.py

+        return str_rep
+
+    @staticmethod
+    def datetime_from_user_input(datetime_obj):


Should we consider dateutil.parser.parse here instead of rolling our own? If so, we could include dateutil as an optional dependency and benefit from any improvements made there. If so, should we use similar terminology and call this method parse instead?

I think we should consider it, but dateutil would need to be a core dependency because ModelTime is constructed much like the Grid classes on modflow model objects.

renamed this to parse_datetime

langevin-usgs · 2024-11-14T22:10:16Z

flopy/discretization/modeltime.py

+
+        return dt
+
+    def intersect(self, datetime_obj=None, totim=None, kper_kstp=False, forgrive=False):


Suggested change

def intersect(self, datetime_obj=None, totim=None, kper_kstp=False, forgrive=False):

def intersect(self, datetime_obj=None, totim=None, kper_kstp=False, forgive=False):

Note that forgive is misspelled in a few places.

Also, I wonder if intersect is the best name for this? Would this be better explained with find?

updated the forgive flag.

We should think about the naming here, because I think that this is going to be one of the more used methods of the class. I'm not sure that find is quite right for the method name

flopy/discretization/modeltime.py

langevin-usgs · 2024-11-20T20:11:33Z

flopy/discretization/modeltime.py

+
+        return datetime_obj
+
+    def get_totim(self, kper, kstp=None):


We have a method here called get_totim which means something different from the totim method. I think people might find this confusing. We should probably try and settle on a terminology, such as start/stop or begin/end. We might also need to think about a way to distinguish between datetime and elapsed time.

How about get_model_time or get_end_time?

how about get_kstp_kper_totim(), since I think that's what it's doing? Also I think it's important to note that simulation time might be different from elapsed time, for example if the simulation starts with a steady-state stress period of some length.

flopy/discretization/modeltime.py

wpbonelli · 2024-11-26T12:28:19Z

flopy/discretization/modeltime.py

    """

    def __init__(
        self,
-        period_data=None,


I know period data will be computed dynamically from perlen/nstp/tsmult now, but maybe still convenient to pass in a dataframe/recarray/list of tuples.. would it be hard to keep support for that?

It's possible to keep support for that, but if we added that too it would create multiple ways to instantiate the class. I personally think we should have a single standard way (either period_data or by each variable that would be in a period_data array). But I'm open to having multiple ways, it just complicates the code

I'd personally vote to keep the initializer parameter a table, for backwards compatibility and because it is consistent with what we do for packages. We could have a from_arrays(perlen, nstp, tsmult) class method?

In future we could maybe also consider e.g. a from_times(times) class method doing what imod-python does

flopy/discretization/modeltime.py

…roperty to get a list of datetimes at the end of each time step

aleaf · 2024-12-19T12:28:47Z

autotest/test_modeltime.py

+    if modeltime.start_datetime != start_datetime:
+        raise AssertionError("start_datetime improperly stored")
+
+    result = modeltime.intersect("3/06/2024 23:59:59")


I know intersect is used elsewhere, but consider renaming this method to say what it does, for example get_kstpkper() (if I'm understanding this correctly)

aleaf · 2024-12-19T16:54:31Z

autotest/test_modeltime.py

+def test_timeunits_userinput_parsing():
+    formats = {
+        "years": ["years", "YeaR", "yaEr", "ayer", "y", "yr", 5],
+        "days": ["days", "Day", "dyAs", "dysa", "d", 4],
+        "hours": ["hours", "Hour", "huors", "h", "hrs", 3],
+        "minutes": ["minutes", "MinUte", "minte", "m", "min", 2],
+        "seconds": ["seconds", "Second", "sedcon", "s", "sec", 1],
+        "unknown": ["unkonwn", "undefined", "u", 0],
+    }
+
+    for unit_name, checks in formats.items():
+        for check in checks:
+            mt_unit = ModelTime.timeunits_from_user_input(check)
+            if mt_unit != unit_name:
+                raise AssertionError(
+                    "Units are unable to be determined from user input"
+                )


consider refactoring test cases like this (here and in the functions below) to use @pytest.mark.parametrize. For example, this test could be refactored to:

@pytest.mark.parametrize('unit_name,user_inputs', ( ("years", ["years", "YeaR", "yaEr", "ayer", "y", "yr", 5]), ("days", ["days", "Day", "dyAs", "dysa", "d", 4]), ("hours", ["hours", "Hour", "huors", "h", "hrs", 3]), ("minutes", ["minutes", "MinUte", "minte", "m", "min", 2]), ("seconds", ["seconds", "Second", "sedcon", "s", "sec", 1]), ("unknown", ["unkonwn", "undefined", "u", 0]) )) def test_timeunits_userinput_parsing(unit_name,user_inputs): for user_input in user_inputs: mt_unit = ModelTime.timeunits_from_user_input(user_input) assert mt_unit == unit_name

This carries some advantages, in that each case is treated as a separate test (meaning all of the cases will be run unless "fail fast" is set), and the tests can be debugged individually in VS Code.

aleaf · 2024-12-19T16:55:38Z

autotest/test_modeltime.py

+    valid = datetime.datetime(2024, 11, 12)
+
+    for dt_rep in formats:
+        dt_obj = ModelTime.parse_datetime(dt_rep)
+        if dt_obj != valid:
+            raise AssertionError("datetime not properly determined from user input")


for simplicity and readability, this could be shortened to simply

assert ModelTime.parse_datetime(dt_rep) == datetime.datetime(2024, 11, 12)

same for other checks with AssertionErrors

aleaf · 2024-12-19T17:11:49Z

flopy/discretization/modeltime.py

+            tsmult = np.full((nrecs,), 1)
+
+        if nrecs != len(nstp):
+            raise AssertionError()


should these be ValueErrors that say what went wrong? For example, "perlen and nstp inputs must have the same length"

aleaf · 2024-12-19T17:20:42Z

flopy/discretization/modeltime.py

+
+        return self._totim_dict[(kper, kstp)]
+
+    def get_datetime(self, kper, kstp=None):


It might be useful to add an option to return the start datetime of a period/timestep instead (leaving end datetime as the default). For example, if we are trying to aggregate higher-frequency or mis-aligned data to the model time discretization by computing averages over the model stress periods. Having easy access to the period start and end dates might also be useful for some visualization/post-processing.

aleaf · 2024-12-19T17:25:29Z

I think that's all I have for now. Thanks for putting this together @jlarsen-usgs. Is there a way to see test coverage or is that turned off for draft requests?

jlarsen-usgs added 3 commits November 13, 2024 14:25

Linting

a80105b

Update NetCdf for ModelTime changes

e77e4cb

* add get_datetime_string() method to ModelTime to format ISO 8601 compliant date times

jlarsen-usgs requested review from langevin-usgs and jdhughes-usgs November 13, 2024 23:27

jlarsen-usgs added 2 commits November 13, 2024 15:43

Remove TemporalReference, replaced functionality with ModelTime

0a85cc1

commit test_copy and test_usg updates

a9ed6ec

langevin-usgs marked this pull request as draft November 14, 2024 17:21

langevin-usgs reviewed Nov 20, 2024

View reviewed changes

wpbonelli reviewed Nov 26, 2024

View reviewed changes

flopy/discretization/modeltime.py Outdated Show resolved Hide resolved

wpbonelli reviewed Nov 26, 2024

View reviewed changes

flopy/discretization/modeltime.py Outdated Show resolved Hide resolved

wpbonelli reviewed Nov 26, 2024

View reviewed changes

flopy/discretization/modeltime.py Outdated Show resolved Hide resolved

wpbonelli reviewed Nov 26, 2024

View reviewed changes

flopy/discretization/modeltime.py Show resolved Hide resolved

wpbonelli reviewed Nov 26, 2024

View reviewed changes

flopy/discretization/modeltime.py Outdated Show resolved Hide resolved

jlarsen-usgs requested a review from aleaf December 4, 2024 16:22

jlarsen-usgs added 2 commits December 17, 2024 14:25

First pass of changes to ModelTime for review comments

f4624a8

Refactors set methods to property setters and added a datetimes p…

b135d69

…roperty to get a list of datetimes at the end of each time step

aleaf reviewed Dec 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update(ModelTime): refactor ModelTime and add new features #2367

Update(ModelTime): refactor ModelTime and add new features #2367

jlarsen-usgs commented Nov 13, 2024

wpbonelli commented Nov 14, 2024

jlarsen-usgs commented Nov 14, 2024

langevin-usgs left a comment

langevin-usgs Nov 14, 2024

jlarsen-usgs Dec 17, 2024 •

edited

Loading

jlarsen-usgs Dec 17, 2024

langevin-usgs Nov 14, 2024

jlarsen-usgs Dec 17, 2024

langevin-usgs Nov 20, 2024

jlarsen-usgs Dec 17, 2024

aleaf Dec 19, 2024

wpbonelli Nov 26, 2024

jlarsen-usgs Dec 17, 2024

wpbonelli Dec 18, 2024

aleaf Dec 19, 2024

aleaf Dec 19, 2024

aleaf Dec 19, 2024

aleaf Dec 19, 2024

aleaf Dec 19, 2024 •

edited

Loading

aleaf commented Dec 19, 2024


		return dt

		def intersect(self, datetime_obj=None, totim=None, kper_kstp=False, forgrive=False):


		return self._totim_dict[(kper, kstp)]

		def get_datetime(self, kper, kstp=None):

Update(ModelTime): refactor ModelTime and add new features #2367

Are you sure you want to change the base?

Update(ModelTime): refactor ModelTime and add new features #2367

Conversation

jlarsen-usgs commented Nov 13, 2024

wpbonelli commented Nov 14, 2024

jlarsen-usgs commented Nov 14, 2024

langevin-usgs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlarsen-usgs Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleaf Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

aleaf commented Dec 19, 2024

jlarsen-usgs Dec 17, 2024 •

edited

Loading

aleaf Dec 19, 2024 •

edited

Loading